Fix some path logic to work on Windows #2704

jchadwick-buf · 2024-01-09T14:04:49Z

This does not fix everything, so Windows CI is still not passing. However, this should still be closer.

I think ExternalPath is supposed to be unnormalized, so the tests that test for it are fixed to use unnormalized paths.

bufdev · 2024-01-09T15:57:52Z

private/buf/buffetch/internal/reader.go

 	osRootBucket, err := storageosProvider.NewReadWriteBucket(
-		string(os.PathSeparator),
+		inputDirPathComponents[0],


This isn't right/isn't what the intended behavior is for unix, can comment later

For sake of posterity this behavior is identical on UNIX to the previous behavior because Components is called on the absolute path and looks like this:

[]string{"/", "Users", "john", "Code", "buf", "private", "buf", "cmd", "buf", "testdata", "fail2"}

So the bucket on UNIX is always "/" but on Windows could be a volume name (drive letter, UNC path.)
I'm not really opposed to refactoring it anyways (this is a pretty ugly approach at the end of the day,) just worth noting.

So I think using normalpath.Components in this way is somewhat reasonable, since it's parsing an absolute path, and so on UNIX, the behaviour is very safe. However, as you noted, the Windows path could be either fully-qualified or UNC, and I think this creates some instability. We do handle the first component in normalpath to set the volume:

buf/private/pkg/normalpath/normalpath_windows.go

Lines 129 to 146 in 21f4b65

if len(volumeComponent) > 0 {

// On Windows the volume of an absolute path could be one of the following 3 forms

// c.f. https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file?redirectedfrom=MSDN#fully-qualified-vs-relative-paths

// * A disk designator: `C:\`

// * A UNC Path: `\\servername\share\`

// c.f. https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-dfsc/149a3039-98ce-491a-9268-2f5ddef08192

// * A "current volume absolute path" `\`

// This refers to the root of the current volume

//

// We do not support paths with string parsing disabled such as

// `\\?\path`

//

// If we did extract a volume name, we need to add a path separator to turn it into

// a path component. Volume Names without path separators have an implied "current directory"

// when performing a join operation, or using them as a path directly, which is not the

// intention of `Split` so we ensure they always mean "the root of this volume".

volumeComponent = volumeComponent + stringOSPathSeparator

}

I think the concern is actually when we're given a UNC path. So I think the better route to go is to explicitly parse the drive and pass that through as our path, instead of deriving it from the arbitrary path structure.

bufdev

Blocking as this isn't what the fix should be unless I'm missing something

doriable · 2024-01-10T23:28:57Z

I put this in draft because I have no resolved all the issues yet, but would like to push up some changes. Will un-draft and request reviews once these issues are resolved.

…ndows-fixes

doriable · 2024-01-11T02:08:29Z

private/pkg/filepathext/filepathext_windows.go

+	// USERPROFILE shows the current user's profile folder. It is being used to parse the volume
+	// for the current user and this is used as the process's context.
+	// https://learn.microsoft.com/en-us/windows/deployment/usmt/usmt-recognized-environment-variables#variables-that-are-processed-for-the-operating-system-and-in-the-context-of-each-user
+	FSRoot = filepath.VolumeName(os.Getenv("USERPROFILE")) + string(os.PathSeparator)


I retract my comment earlier, I think it isn't feasible to parse this through the system env vars -- the process could just be running in an entirely separate drive, which is what is happening in the actions. We should use normalpath.Components when parsing the absolute paths and document clearly the behaviours of how reader.go is constructing the OS root buckets.

bufdev · 2024-01-16T16:35:31Z

private/bufpkg/bufconfig/file.go

@@ -205,13 +206,15 @@ func newDecodeError(fileName string, err error) error {
 		fileName = "config file"
 	}
 	// We intercept PathErrors in buffetch to deal with fixing of paths.
-	return &fs.PathError{Op: "decode", Path: fileName, Err: err}
+	// We return a cleaned, unnormalized path in the error for clarity with user's filesystem.
+	return &fs.PathError{Op: "decode", Path: filepath.Clean(normalpath.Unnormalize(fileName)), Err: err}


fileName isn't necessarily a path I believe

In the case where it isn't a path (e.g. fileName = "config file" or some other case), normalpath.Unnormalize and filepath.Clean would be no-ops.

We need to make a decision as to whether paths should be unnormalized in fs.PathErrors - I've generally taken the position that they should not be in the rest of the codebase, I'm worried that we're special-casing here, any thoughts?

I'm fine with either -- the unnormalized error was mostly for UX reasons (matching file paths, and we did show unnormalised file paths in the past, etc.) but as noted, they're not always a file path, and consistency is kind of the key. So I'm totally okay with keeping them as normalised paths if this is the pattern throughout the new libraries, in which case, I can adjust the tests to reflect that also.

…ndows-fixes

doriable

Added TODOs for all WIndows tests not fixed here:
- Formatter diffs
- b5 digest issues
- Duplicate path separators, even after filepath.Clean
- Carriage returns affecting output
- Archive test concurrency

…ndows-fixes

doriable · 2024-01-17T21:38:08Z

Turns out a large portion of our issues were due to a .gitattributes change that did not allow git to convert line endings on checkout for testdata. In the past, we set **/testdata/** files to binary, which forces git to treat them as binary.
The change set -crlf instead, but this assumes that git does not check them out as text files.
So now, we are setting -text explicitly so that they are not being handled by text=auto setting, even if they are being checked out as text files. This fixes many of the issues caused by carriage returns.

There are still a couple of remaining TODO's to investigate:

duplicated path separators
archive test concurrency issues

…ndows-fixes

…t runs

bufdev

@doriable and I chatted and we're going to merge this in as-is, and @doriable is going to fix up the remaining tests on a new PR.

Fix some path logic to work on Windows

c18b611

jchadwick-buf requested review from bufdev and doriable January 9, 2024 14:04

bufdev reviewed Jan 9, 2024

View reviewed changes

bufdev requested changes Jan 9, 2024

View reviewed changes

doriable marked this pull request as draft January 10, 2024 23:12

doriable added 2 commits January 10, 2024 18:17

Set a FSRoot in filepathext package

85f9e39

Run make generate

4d88b2a

doriable added 7 commits January 10, 2024 19:32

Small fixes to test

a106cf0

Some more fixes

1f88005

Merge remote-tracking branch 'origin/bufmod' into jchadwick/bufmod-wi…

b5c666e

…ndows-fixes

More reader_test assertion fixes

ffdf1ef

Need to parse components in order to get relative input path

34ce447

Fix absolute path tests for Windows workspaces

c64f727

Attempt to use USERPROFILE to get volume of current process

3df5d92

doriable reviewed Jan 11, 2024

View reviewed changes

Pearlminimic99 approved these changes Jan 11, 2024

View reviewed changes

doriable added 6 commits January 11, 2024 14:57

Revert back to using normalpath.Components fix and add comments

6003241

Fix some low hanging fruit to unnormalize file paths in error returns

3ac2242

Add filepath.Clean to decode/encode errors

07041e7

A couple more clean-ups

fd39088

More fixes

c93de2f

More simple fixes

fa317be

bufdev reviewed Jan 16, 2024

View reviewed changes

doriable added 5 commits January 16, 2024 12:48

Add TODOs for archive test failures

0caa8ed

Change exclude path error

7334382

Merge remote-tracking branch 'origin/bufmod' into jchadwick/bufmod-wi…

51e85b3

…ndows-fixes

Add skips for formatter and bufimageutil/generate tests

ebe2579

Add skips for storagetesting package for carriage returns

ea950f7

Add more TODOs

37740e7

doriable reviewed Jan 16, 2024

View reviewed changes

doriable marked this pull request as ready for review January 16, 2024 22:29

Merge remote-tracking branch 'origin/bufmod' into jchadwick/bufmod-wi…

1fe3539

…ndows-fixes

jchadwick-buf mentioned this pull request Jan 17, 2024

Add LSP server #2662

Closed

doriable added 2 commits January 17, 2024 16:30

Change the .gitattributes to -text -diff for testdata files

2bc8859

Ungate bufimageutil tests

8ecf668

doriable added 4 commits January 17, 2024 16:39

Ungate generate test

66a0f04

Merge remote-tracking branch 'origin/bufmod' into jchadwick/bufmod-wi…

bb6c42f

…ndows-fixes

Use t.TempDir() for archive tests for unique temp directories for tes…

c5b8cb9

…t runs

Skip invalid path archive tests

d8924ce

bufdev approved these changes Jan 18, 2024

View reviewed changes

bufdev merged commit 606fc67 into bufmod Jan 18, 2024
7 checks passed

bufdev deleted the jchadwick/bufmod-windows-fixes branch January 18, 2024 17:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some path logic to work on Windows #2704

Fix some path logic to work on Windows #2704

jchadwick-buf commented Jan 9, 2024

bufdev Jan 9, 2024

jchadwick-buf Jan 10, 2024 •

edited

Loading

doriable Jan 10, 2024

bufdev left a comment

doriable commented Jan 10, 2024

doriable Jan 11, 2024

bufdev Jan 16, 2024

doriable Jan 16, 2024

bufdev Jan 17, 2024

doriable Jan 17, 2024

doriable left a comment

doriable commented Jan 17, 2024 •

edited

Loading

bufdev left a comment

	if len(volumeComponent) > 0 {
	// On Windows the volume of an absolute path could be one of the following 3 forms
	// c.f. https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file?redirectedfrom=MSDN#fully-qualified-vs-relative-paths
	// * A disk designator: `C:\`
	// * A UNC Path: `\\servername\share\`
	// c.f. https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-dfsc/149a3039-98ce-491a-9268-2f5ddef08192
	// * A "current volume absolute path" `\`
	// This refers to the root of the current volume
	//
	// We do not support paths with string parsing disabled such as
	// `\\?\path`
	//
	// If we did extract a volume name, we need to add a path separator to turn it into
	// a path component. Volume Names without path separators have an implied "current directory"
	// when performing a join operation, or using them as a path directly, which is not the
	// intention of `Split` so we ensure they always mean "the root of this volume".
	volumeComponent = volumeComponent + stringOSPathSeparator
	}

Fix some path logic to work on Windows #2704

Fix some path logic to work on Windows #2704

Conversation

jchadwick-buf commented Jan 9, 2024

bufdev Jan 9, 2024

Choose a reason for hiding this comment

jchadwick-buf Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

doriable Jan 10, 2024

Choose a reason for hiding this comment

bufdev left a comment

Choose a reason for hiding this comment

doriable commented Jan 10, 2024

doriable Jan 11, 2024

Choose a reason for hiding this comment

bufdev Jan 16, 2024

Choose a reason for hiding this comment

doriable Jan 16, 2024

Choose a reason for hiding this comment

bufdev Jan 17, 2024

Choose a reason for hiding this comment

doriable Jan 17, 2024

Choose a reason for hiding this comment

doriable left a comment

Choose a reason for hiding this comment

doriable commented Jan 17, 2024 • edited Loading

bufdev left a comment

Choose a reason for hiding this comment

jchadwick-buf Jan 10, 2024 •

edited

Loading

doriable commented Jan 17, 2024 •

edited

Loading